-
-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add new filter to filter to an enum #94
base: 2.31.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Reinfi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good direction! Needs some feedback from @gsteel too, but overall nice idea to start handling ENUMs directly 👍
src/ToEnum.php
Outdated
use function is_subclass_of; | ||
|
||
/** | ||
* @psalm-type Options array{enum: class-string<UnitEnum>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should keep extending AbstractFilter
at all, but that's something @gsteel should help us decide.
In this case, new ToEnum(MyEnum::class)
seems cleaner to me, and doesn't really need the API inherited from AbstractFilter
, although unsure if divergence from that is better or worse.
Adding AbstractFilter
can also be done afterwards too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that would not work with the factory? Or should I add a factory to handle the case?
Extending AbstractFilter could be easily removed because I do not really need it.
src/ToEnum.php
Outdated
class ToEnum extends AbstractFilter | ||
{ | ||
/** @var Options */ | ||
protected $options = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The psalm errors detected here should be similar to laminas/laminas-validator#168
Perhaps polyfilling with a .phpstub
file in Psalm could suffice.
src/ToEnum.php
Outdated
/** | ||
* @param class-string<UnitEnum>|Traversable|Options $enumOrOptions | ||
*/ | ||
public function __construct($enumOrOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love for this to only accept class-string<UnitEnum>
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor arg needs to be
public function __construct($enumOrOptions) | |
public function __construct($enumOrOptions = []) |
or
public function __construct($enumOrOptions) | |
public function __construct($enumOrOptions = null) |
(null would also need to be added to the @param
in this case)
The FilterPluginManager
will just new
the filter without arguments unless a factory is created (which is pointless).
In order for the filter to be available in the plugin manager, FilterPluginManager
must be updated with entries under factories
and aliases
The problem is that at this point, PHPUnit will fail CI on PHP 8.0 (Along with Psalm)
I don't see an easy way around this at the moment unless dropping support for PHP 8.0 is an option.
Thoughts @Ocramius ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FilterPluginManager will just new the filter without arguments unless a factory is created…
Correct! 👍🏻
src/ToEnum.php
Outdated
/** | ||
* @psalm-type Options array{enum: class-string<UnitEnum>} | ||
* @extends AbstractFilter<Options> | ||
*/ | ||
class ToEnum extends AbstractFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be made more generic:
/**
* @template TEnum of UnitEnum
*/
Then in the constructor you can:
/**
* @param class-string<TEnum> $enum
*/
public function __construct(string $enum)
{
// ...
}
This way, filter()
becomes generic, and you can infer the correct ENUM type too:
/**
* @return TEnum|null
*/
public function filter(mixed $value): ?UnitEnum
{
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but psalm will not understand my type
psalm: UndefinedDocblockClass: Docblock-defined class, interface or enum named Laminas\Filter\TEnum does not exist
when I use class-string<TEnum>|Traversable|Options
. If I remove the Traversable|Options
psalm understands it.
Signed-off-by: Reinfi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice addition :)
I'm wondering whether we can add phpVersion=8.1
to psalm.xml
to silence most of the psalm issues here: https://psalm.dev/docs/running_psalm/configuration/#phpversion
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
When psalm is running on php8.1 everything is now working. |
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
Signed-off-by: Reinfi <[email protected]>
Description
I added a filter to filter a given value to an enum. Is a bit like the pull request here.
I'm unsure if the filter should return the original value if it is not in the enum or null. I actually added it as null because than you can be sure that your value is either null or the expected enum.
Hopefully I added everything related to your contribution guidelines.
Of course this filter only works > PHP 8.0.
And I think psalm will fail for my new class, but hopefully someone of you can help me how to solve it.